Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Modify http and grpc proxy to receive a listener rather than creating it #321

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Modify http and grpc proxy to receive a listener rather than creating it #321

merged 2 commits into from
Sep 13, 2023
+194 −280

Conversation

roobre
Copy link
Member

@roobre roobre commented Aug 24, 2023 •
edited
Loading

Description

This PR changes HTTP an GRPC proxy constructors to receive a listener as an argument, instead of receiving an address and creating it themselves as they used to.

Unblocks: #314

Doing this allows to move most of the startup logic to the constructor. Doing so, removes the potential race condition between Start and Stop.
Fixes: #319

Fixes # (issue)

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make test) and all tests pass.
  • I have run relevant e2e test locally (make e2e-xxx for agent, disruptors, kubernetes or cluster related changes)
  • Any dependent changes have been merged and published in downstream modules

@roobre
Copy link
Member Author

roobre commented Aug 24, 2023

As an interesting side-effect, the following e2e test is returning a different error now:

--- FAIL: Test_Agent (116.51s)
    --- FAIL: Test_Agent/Prevent_execution_of_multiple_commands (21.85s)
        agent_e2e_test.go:328: unexpected error: setting up listener at ":8080": listen tcp :8080: bind: address already in use
            :
FAIL
FAIL	github.com/grafana/xk6-disruptor/e2e/agent	143.144s
FAIL
make: *** [Makefile:36: e2e-agent] Error 1

I believe the reason for this is simply that before, ApplyDisruption was called before the proxy started the listener.

@roobre roobre force-pushed the proxy-net-listener branch from d77ffe4 to e94ef36 Compare August 31, 2023 13:11
@roobre
Copy link
Member Author

roobre commented Aug 31, 2023

After changes in 1a46933, E2E are passing fine, the error regression is no longer there. Ready to review.

@roobre roobre marked this pull request as ready for review August 31, 2023 15:27
roobre
roobre commented Aug 31, 2023
View reviewed changes
pkg/agent/agent.go Outdated Show resolved Hide resolved
@roobre
Copy link
Member Author

roobre commented Sep 13, 2023

Commits squashed, I'll merge after a final pass of e2e tests passes.

@roobre roobre merged commit f3324fa into main Sep 13, 2023
6 checks passed
@roobre roobre deleted the proxy-net-listener branch September 13, 2023 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablochacin pablochacin pablochacin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test-scoped data race in GRPC tests
2 participants
@roobre @pablochacin